Support commit-specific code review (支持指定commit审查)#5
Conversation
Co-authored-by: zzy2210 <44086018+zzy2210@users.noreply.github.com>
Co-authored-by: zzy2210 <44086018+zzy2210@users.noreply.github.com>
Co-authored-by: zzy2210 <44086018+zzy2210@users.noreply.github.com>
Co-authored-by: zzy2210 <44086018+zzy2210@users.noreply.github.com>
There was a problem hiding this comment.
Pull Request Overview
This PR implements the previously defined but non-functional --commit-id flag to enable code review of specific commit changes rather than just working directory changes.
- Added commit-specific diff functionality using go-git's
object.DiffTree()method - Enhanced the review engine constructor to accept and store a commit ID parameter
- Modified CLI command handling to pass the commit ID flag to the review engine
- Added improved error messaging and reporting features
Reviewed Changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| internal/reviewer/reviewer.go | Implements core commit diff functionality with new methods and enhanced reporting |
| cmd/stellarspec.go | Updates CLI to pass commit ID parameter and improves error messaging |
| if fileName == "" { | ||
| fileName = change.From.Name | ||
| } | ||
| if fileName == "go.sum" || fileName == "go.mod" || strings.Contains(fileName, "README") { |
There was a problem hiding this comment.
The file filtering logic is duplicated between getCommitDiff() and getWorkingTreeDiff(). Consider extracting this logic into a separate helper function to avoid code duplication.
| if fileName == "go.sum" || fileName == "go.mod" || strings.Contains(fileName, "README") { | |
| if shouldSkipFile(fileName) { |
| parentCommit, err := commit.Parent(0) | ||
| if err != nil { | ||
| // 如果无法获取父 commit,可能是初始 commit,我们就与空 tree 比较 | ||
| fmt.Printf("Warning: Could not get parent commit (this might be an initial commit): %v\n", err) |
There was a problem hiding this comment.
[nitpick] Consider using a proper logger instead of fmt.Printf for warning messages. This would provide better control over log levels and output formatting.
| fmt.Printf("Warning: Could not get parent commit (this might be an initial commit): %v\n", err) | |
| log.Printf("WARNING: Could not get parent commit (this might be an initial commit): %v", err) |
| // 生成 patch | ||
| patch, err := change.Patch() | ||
| if err != nil { | ||
| fmt.Printf("failed to get patch for file: path= %s, err= %v \n", fileName, err) |
There was a problem hiding this comment.
[nitpick] Consider using a proper logger instead of fmt.Printf for error messages. This would provide better control over log levels and output formatting.
| fmt.Printf("failed to get patch for file: path= %s, err= %v \n", fileName, err) | |
| log.Printf("failed to get patch for file: path= %s, err= %v \n", fileName, err) |
| baseConf, err := config.LoadFile(configPath) | ||
| if err != nil { | ||
| fmt.Println("load config file failed: err= %v", err) | ||
| fmt.Printf("load config file failed: err= %v\n", err) |
There was a problem hiding this comment.
The error message format is inconsistent. Consider using standard Go error formatting: 'failed to load config file: %v' instead of 'load config file failed: err= %v'.
| fmt.Printf("load config file failed: err= %v\n", err) | |
| fmt.Printf("failed to load config file: %v\n", err) |
Implements the ability to review changes from specific commits using the existing
--commit-idflag, which was previously defined but not functional.Changes Made
Core Implementation:
commitIDfield toReviewEnginestruct to store the target commitNewReviewEngine()constructor to acceptcommitIDparameter--commit-idflag value to the review enginegitDiff()method into specialized functions:getCommitDiff(): Compares specified commit with its parent usingobject.DiffTree()getWorkingTreeDiff(): Preserves original working directory comparison logicEnhanced Features:
Usage Examples
Testing
The implementation has been tested with:
Technical Details
The implementation uses go-git's
object.DiffTree()function to efficiently compare the specified commit with its parent, generating proper diff content that can be analyzed by the LLM. When no parent exists (initial commit), it compares against an empty tree to show all files as additions.Fixes #1.
💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.